fix: clean matched vars after chained and non-chained rule #3418
Conversation
|
|
@mirkodziadzka-avi could you take a review on this? |
Great, thanks!
Indeed, after the merge I'm going to update it. Thank you! |
| @@ -355,6 +355,9 @@ bool RuleWithOperator::evaluate(Transaction *trans, | |||
|
|
|||
| /* last rule in the chain. */ | |||
| performLogging(trans, ruleMessage, true, true); | |||
| if (m_ruleId > 0) { | |||
There was a problem hiding this comment.
Why this check? If I understand correctly, m_ruleId == 0 would be an invalid rule (caught exception). Would we even get here then? As far as I can tell, nothing bad would happen if m_ruleId == 0, since cleanMatchedVars() operates on the transaction only.
There was a problem hiding this comment.
If I understand correctly,
m_ruleId == 0would be an invalid rule
Yes, except if it's a chained rule. Generally, every chained rule (rules?) has (have?) only one unique id. In libmodsecurity3, despite you set up the id action at the first rule (in a chained rule), the last rule will own that id. Therefore this condition (m_ruleId > 0) tells us this is the end of a rule, no matter that's chained or not, we should clean the MATCHED_* variables.
what
This PR changes the code behavior: now the engine cleans the
MATCHED_VAR*variables after chained and non-chained rules too.why
Until now if there was a single (non-chained) rule, and if any of the
MATCHED_VAR*variable were filled, then the next rule which used them accessed the filled value, even the rule does not usechainaction.references
See issue #3382.
This PR fixes #3382.
other notes
please see commit 5572ac0; I added this change because the first test on Windows was failed. It seems like the argument processing order is non-deterministic, at least it's different on Windows (see the log: the first argument is the last from the
QUERY_STRING, and the tests were success on all other platforms).